plan: fix panic caused by empty schema of LogicalTableDual #7906

tianjiqx · 2018-10-15T15:24:49Z

What problem does this PR solve?

fix #7833

What is changed and how it works?

Initialize null pointer

Check List

Tests

Unit test

Code changes

Has exported function/method change

Side effects

Related changes

Need to cherry-pick to the release branch

sre-bot · 2018-10-15T15:24:53Z

Hi contributor, thanks for your PR.

This patch needs to be approved by someone of admins. They should reply with "/ok-to-test" to accept this PR for running test automatically.

CLAassistant · 2018-10-15T15:24:57Z

All committers have signed the CLA.

shenli · 2018-10-15T16:22:24Z

/ok-to-test

shenli · 2018-10-15T16:22:51Z

@tianjiqx Thanks! Please add a unit test case.

shenli · 2018-10-15T16:23:02Z

/run-all-tests

eurekaka · 2018-10-16T02:07:27Z

I thinks we should not enforce LogicalTableDual to have a valid schema, because semantically speaking, it has no such constraint, instead I prefer to add a null check in LogicalTableDual::PruneColumns for p.schema, like say:

func (p *LogicalTableDual) PruneColumns(parentUsedCols []*expression.Column) {
    if p.schema == nil {
        return
    }
    xxx
}

If we choose to enforce valid schema of LogicalTableDual, we have to double check all existing LogicalTableDual in current code base then.

eurekaka · 2018-10-16T02:08:29Z

planner/core/preprocess_test.go

@@ -194,6 +194,9 @@ func (s *testValidatorSuite) TestValidator(c *C) {
 		{"select * from ( select 1 ) a, (select 2) b;", true, nil},
 		{"select * from (select * from ( select 1 ) a join (select 2) b) b join (select 3) a;", false, nil},
 		{"select * from (select 1 ) a , (select 2) b, (select * from (select 3) a join (select 4) b) c;", false, nil},
+
+		//issue 7833
+		{"drop view if exists v;", true, nil},


Better to add this test in TestColumnPruning.

Well, I found that this test case can be passed here（ planner/core/preprocess_test.go）, and even without fixing the bug, I'm confused, maybe I don't fully understand the tidb test.

eurekaka · 2018-10-16T02:14:26Z

Kind reminder, this PR need to be cherry-picked into release-2.1 branch after merged into master. release-2.0 branch does not have this problem.

zz-jason · 2018-10-16T03:08:20Z

planner/core/planbuilder.go

@@ -227,6 +227,8 @@ func (b *PlanBuilder) buildDo(v *ast.DoStmt) (Plan, error) {
 			RetType:  expr.GetType(),
 		})
 	}
+	//make dual schema if not
+	var _ = dual.Schema()


how about directly set the schema of the dual operator, which eliminates the confusion:

dual.schema = expression.NewSchema()

Yes, it looks better

zz-jason

LGTM

zz-jason · 2018-10-16T06:53:56Z

@eurekaka @lamxTyler @lzmhhh123 PTAL

zz-jason · 2018-10-16T06:54:09Z

/run-all-tests

XuHuaiyu

LGTM

bugfix fixed pingcap#7518 expression: MySQL compatible current_user function (pingcap#7801) plan: propagate constant over outer join (pingcap#7794) - extract `outerCol = const` from join conditions and filter conditions, substitute `outerCol` in join conditions with `const`; - extract `outerCol = innerCol` from join conditions, derive new join conditions based on this column equal condition and `outerCol` related expressions in join conditions and filter conditions; util/timeutil: fix data race caused by forgetting set stats lease to 0 (pingcap#7901) stats: handle ddl event for partition table (pingcap#7903) plan: implement Operand and Pattern of cascades planner. (pingcap#7910) planner: not convert to TableDual if empty range is derived from deferred constants (pingcap#7808) plan: move projEliminate behind aggEliminate (pingcap#7909) admin: fix admin check table bug of byte compare (pingcap#7887) * admin: remove reflect deepEqual stats: fix panic caused by empty histogram (pingcap#7912) plan: fix panic caused by empty schema of LogicalTableDual (pingcap#7906) * fix drop view if exist error (pingcap#7833) executor: refine `explain analyze` (pingcap#7888) executor: add an variable to compatible with MySQL insert for OGG (pingcap#7863) expression: maintain `DeferredExpr` in aggressive constant folding. (pingcap#7915) stats: fix histogram boundaries overflow error (pingcap#7883) ddl:support the definition of `null` change to `not null` using `alter table` (pingcap#7771) * ddl:support the definition of null change to not null using alter table ddl: add check when create table with foreign key. (pingcap#7885) * ddl: add check when create table with foreign key planner: eliminate if null on non null column (pingcap#7924) executor: fix a bug in point get (pingcap#7934) planner, executor: refine ColumnPrune for LogicalUnionAll (pingcap#7930) executor: fix panic when limit is too large (pingcap#7936) ddl: add TiDB version to metrics (pingcap#7902) stats: limit the length of sample values (pingcap#7931) vendor: update tipb (pingcap#7893) planner: support the Group and GroupExpr for the cascades planner (pingcap#7917) store/tikv: log more information when other err occurs (pingcap#7948) types: fix date time parse (pingcap#7933) ddl: just print error message when ddl job is normal to calcel, to eliminate noisy log (pingcap#7875) stats: update delta info for partition table (pingcap#7947) explaintest: add explain test for partition pruning (pingcap#7505) util: move disjoint set to util package (pingcap#7950) util: add PreAlloc4Row and Insert for Chunk and List (pingcap#7916) executor: add the slow log for commit (pingcap#7951) expression: add builtin json_keys (pingcap#7776) privilege: add USAGE in `show grants` for mysql compatibility (pingcap#7955) ddl: fix invailid ddl job panic (pingcap#7940) *: move ast.NewValueExpr to standalone parser_driver package (pingcap#7952) Make the ast package get rid of the dependency of types.Datum server: allow cors http request (pingcap#7939) *: move `Statement` and `RecordSet` from ast to sqlexec package (pingcap#7970) pr suggestion update executor/aggfuncs: split unit tests to corresponding file (pingcap#7993) store/tikv: fix typo (pingcap#7990) executor, planner: clone proj schema for different children in buildProj4Union (pingcap#7999) executor: let information_schema be the first database in ShowDatabases (pingcap#7938) stats: use local feedback for partition table (pingcap#7963) executor: add unit test for aggfuncs (pingcap#7966) server: add log for binary execute statement (pingcap#7987) admin: refine admin check decoder (pingcap#7862) executor: improve wide table insert & update performance (pingcap#7935) ddl: fix reassigned partition id in `truncate table` does not take effect (pingcap#7919) fix reassigned partition id in truncate table does not take effect add changelog for 2.1.0 rc4 (pingcap#8020) *: make parser package dependency as small as possible (pingcap#7989) parser: support `:=` in the `set` syntax (pingcap#8018) According to MySQL document, `set` use the = assignment operator, but the := assignment operator is also permitted stats: garbage collect stats for partition table (pingcap#7962) docs: add the proposal for the column pool (pingcap#7988) expression: refine built-in func truncate to support uint arg (pingcap#8000) stats: support show stats for partition table (pingcap#8023) stats: update error rate for partition table (pingcap#8022) stats: fix estimation for out of range point queries (pingcap#8015) *: move parser to a separate repository (pingcap#8036) executor: fix wrong result when index join on union scan. (pingcap#8031) Do not modify Plan of dataReaderBuilder directly, because it would impact next batch of outer rows, as well as other concurrent inner workers. Instead, build a local child builder to store the child plan. planner: fix a panic of a cached prepared statement with IndexScan (pingcap#8017) *: fix the issue of executing DDL after executing SQL failure in txn (pingcap#8044) * ddl, executor: fix the issue of executing DDL after executing SQL failure in txn add unit test remove debug info add like evaluator case sensitive test ddl, domain: make schema correct after canceling jobs (pingcap#7997) unit test fix code format proposal: maintaining histograms in plan. (pingcap#7605) support _tidb_rowid for table scan range (pingcap#8047) var rename fix

fix drop view if exist error (pingcap#7833)

3a15439

shenli added the contribution This PR is from a community contributor. label Oct 15, 2018

add unit test for drop view

4c1615d

eurekaka reviewed Oct 16, 2018

View reviewed changes

eurekaka changed the title ~~fix drop view if exist error (#7833)~~ plan: fix panic caused by empty schema of LogicalTableDual Oct 16, 2018

eurekaka added sig/planner SIG: Planner type/bugfix This PR fixes a bug. labels Oct 16, 2018

zz-jason reviewed Oct 16, 2018

View reviewed changes

elegant fix drop view panic

81cf95f

tianjiqx dismissed a stale review via 81cf95f October 16, 2018 06:51

zz-jason reviewed Oct 16, 2018

View reviewed changes

fix issue number

eb37f22

tianjiqx force-pushed the bugfix_7833 branch from 7761d84 to eb37f22 Compare October 16, 2018 06:57

XuHuaiyu approved these changes Oct 16, 2018

View reviewed changes

Merge branch 'master' into bugfix_7833

4d3cbdc

XuHuaiyu added status/all tests passed status/LGT2 Indicates that a PR has LGTM 2. labels Oct 16, 2018

tianjiqx and others added 3 commits October 16, 2018 16:29

Merge branch 'master' into bugfix_7833

de3e4d9

Merge branch 'master' into bugfix_7833

06ea882

Merge branch 'master' into bugfix_7833

b612b6f

ngaut merged commit de6b582 into pingcap:master Oct 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

plan: fix panic caused by empty schema of LogicalTableDual #7906

plan: fix panic caused by empty schema of LogicalTableDual #7906

tianjiqx commented Oct 15, 2018 •

edited by eurekaka

Loading

sre-bot commented Oct 15, 2018

CLAassistant commented Oct 15, 2018 •

edited

Loading

shenli commented Oct 15, 2018

shenli commented Oct 15, 2018

shenli commented Oct 15, 2018

eurekaka commented Oct 16, 2018

eurekaka Oct 16, 2018

tianjiqx Oct 16, 2018 •

edited

Loading

eurekaka commented Oct 16, 2018

zz-jason Oct 16, 2018

tianjiqx Oct 16, 2018

zz-jason left a comment

zz-jason commented Oct 16, 2018

zz-jason commented Oct 16, 2018

XuHuaiyu left a comment

plan: fix panic caused by empty schema of LogicalTableDual #7906

plan: fix panic caused by empty schema of LogicalTableDual #7906

Conversation

tianjiqx commented Oct 15, 2018 • edited by eurekaka Loading

What problem does this PR solve?

What is changed and how it works?

Check List

sre-bot commented Oct 15, 2018

CLAassistant commented Oct 15, 2018 • edited Loading

shenli commented Oct 15, 2018

shenli commented Oct 15, 2018

shenli commented Oct 15, 2018

eurekaka commented Oct 16, 2018

eurekaka Oct 16, 2018

Choose a reason for hiding this comment

tianjiqx Oct 16, 2018 • edited Loading

Choose a reason for hiding this comment

eurekaka commented Oct 16, 2018

zz-jason Oct 16, 2018

Choose a reason for hiding this comment

tianjiqx Oct 16, 2018

Choose a reason for hiding this comment

zz-jason left a comment

Choose a reason for hiding this comment

zz-jason commented Oct 16, 2018

zz-jason commented Oct 16, 2018

XuHuaiyu left a comment

Choose a reason for hiding this comment

tianjiqx commented Oct 15, 2018 •

edited by eurekaka

Loading

CLAassistant commented Oct 15, 2018 •

edited

Loading

tianjiqx Oct 16, 2018 •

edited

Loading